-
-
Notifications
You must be signed in to change notification settings - Fork 719
Simplify ruff configuration #41259
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Simplify ruff configuration #41259
Conversation
tobiasdiez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, this is a nice improvement. Especially the handling of the rules activated in CI is way cleaner now.
I have a couple of remarks, but nothing serious.
|
|
||
| # Imports at top-level of file - we can't follow this because we intentionally defer imports for various reasons. | ||
| # maybe once we can use PEP 810 this can be fixed. | ||
| "PLC0415", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are actually quite a few imports in methods that could be moved to the top level without a problem. You want method-level imports only in one instance, namely to break circular imports, right? In that case it would be very helpful if the important is annotated to ease migrating to the new lazy imports (which doesn't help with circular imports) and so it's good if they have a comment like
# noqa: PLC0415 - breaks circular import x > y > z
Then during the migration to the built-in lazy imports, you know you don't need to touch those imports.
If it's just a heavy import that is rarely used in the module, it's okay to convert it to a lazy_import (and then later to the built-in lazy import).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case it would be very helpful if the important is annotated to ease migrating to the new lazy imports (which doesn't help with circular imports)
Lazy imports can help with circular imports depending on the situation. Sometimes a lazy import can fix a circular import, sometimes it can't.
As for what to do until we can use built-in lazy imports, there are so many linter failures related to imports that I think trying to address is futile unless we can reliably do it automatically, I don't think we can until built-in lazy imports are available.
If it's just a heavy import that is rarely used in the module, it's okay to convert it to a lazy_import (and then later to the built-in lazy import).
Yeah but I don't think it's worth the trouble of fixing thousands of lines for this, just to do it again once built-in lazy imports are available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For context: PLC0415 currently has 10606 failures and has no autofix option available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of weeks ago, I looked into how the migration to lazy import will most likely look like for us (and asked some clarifying questions to the PEP authors). Python will provide some way to call the underlying logic of the lazy import mechanism (similar to the dynamic import method). We can use this in our lazy_import method using something like
def lazy_import(module):
if python >= 3.15: return python_builtin_lazy_import(module)
else: our custom implementation
Then, once we have 3.15 as the minimal Python version (which will be in 4 to 5 years), we can replace lazy_import(...) by lazy import ... globally.
So it's a pretty long time frame until we can really profit from the new lazy imports syntax.
As for the local imports, the overall migration to lazy imports would look like:
- Local import > global
lazy_import lazy_import>lazy import
Local imports that cannot be migrated to lazy imports, would need to be annotated in either case.
As for the reason why I like to keep PCL0415 activated is that IDEs highlight the import as a linter warning. So if you touch the surrounding code, you get a natural push to move the local import to a global (lazy) import. Then you usually run the doctests anyway, and will see if the import can be global or it really needs to be local (to break a circular import). I like to keep that nudge, as we would like to remove all possible local imports in the long term anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it's a pretty long time frame until we can really profit from the new lazy imports syntax.
Yeah but given the 10000+ failures of this rule currently, I think there are more important things that take less effort to fix with regards to our code quality.
As for the reason why I like to keep
PCL0415activated is that IDEs highlight the import as a linter warning. So if you touch the surrounding code, you get a natural push to move the local import to a global (lazy) import. Then you usually run the doctests anyway, and will see if the import can be global or it really needs to be local (to break a circular import). I like to keep that nudge, as we would like to remove all possible local imports in the long term anyway.
Sure. I can remove PCL0415 from the pyproject.toml ignore and move it to the CI ignore.
I think we can leave E402 disabled though, having a module-level import not at the top of the file is weird enough that I think any violations of the rule are probably just a case of mixing import and lazy_import statements. I think most of the violations of that rule are in places like all.py files that for whatever reason have a mix of regular and lazy imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I can remove PCL0415 from the pyproject.toml ignore and move it to the CI ignore.
Before I do this: I don't think linters and type checkers can recognize imports from our custom lazy_import, can they? So wouldn't replacing function imports with lazy_import limit our ability to do linting and type checking?
|
|
||
| # Module level import not at top of file. | ||
| # ruff is not aware of our lazy_imports, we may be able to remove this ignore after adopting PEP 810 | ||
| "E402", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand the lazy_import argument. Can you not just have all normal imports at the top, then the lazy imports afterwards?
(We might want to defer this rule until there is a decision on how lazy import should be grouped/sorted - if they get a separate group anyway, that would be a good reason to already follow E402; if they are not grouped separately, then it would only introduce additional noise to first follow E402).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand the
lazy_importargument. Can you not just have all normal imports at the top, then the lazy imports afterwards?
Sure, but I don't think it's worth the amount of effort to do that reformatting and then do it again if we decide to replace our custom lazy import system with the PEP 810 one.
|
Documentation preview for this PR (built with commit c17ef62; changes) is ready! 🎉 |
|
Updated part of the docs: https://doc-pr-41259--sagemath.netlify.app/html/en/developer/tools#ruff |
Also fix a few rules that were only violated a small number of times.
This PR:
src/tox.inito remove duplicationpyproject.tomlE,PL) we list the rules explicitly instead of enabling allEor allPLrules.EandPLrules that we don't want to follow because the hope is that once theEandPLrules are fully stable inruffwe will just enable them all and selectively disable the ones we don't want..github/workflows/ruff.tomlthat starts with thepyproject.tomlconfiguration (usingextend, so no duplication), and disables all rules that currently fail.The benefits of this are:
pyproject.toml) that needs to be modified to add new rulesruff --config .github/workflows/ruff.toml📝 Checklist
⌛ Dependencies